feat(conversations): export conversations as Markdown and PDF - #531
Conversation
21c1830 to
7ce0c2b
Compare
7ce0c2b to
48ff86f
Compare
ewen-poch
left a comment
There was a problem hiding this comment.
Thank you for the thoughtful, security-conscious implementation and the extensive test coverage. The normalized public export model, isolated PDF renderer, and desktop/web boundary are all valuable additions.
I found the following issues that should be addressed before merge:
-
The production active-session guard is ineffective. In
src/main/session-persistence/conversation-export.ts:95-99, the service checks the status returned byloadSession. In production this isSessionRepository.loadSession(), which goes throughnormalizeSessionFile() -> normalizeSessionAfterRestore()and converts persistedrunningandwaiting-permissionsessions toerrorbefore returning them. As a result, a stale renderer action or IPC call can export an incomplete active conversation, even though the mocked service test passes. Please check an authoritative live status or use a durable read that does not apply restart normalization, and add an integration test using the real repository. -
An unfinished reasoning block can leak into a public export.
sanitizeExportMarkdownremoves only closed<think>...</think>pairs, and the new test explicitly expectsbefore <think>unfinishedto remain. This conflicts with the acceptance criterion that thinking content is omitted, especially for errored/interrupted responses. Please strip an unmatched assistant<think>opener through EOF as well, while continuing to preserve user-authored examples. -
Prompt-derived document titles are still truncated.
deriveTitleFromPromptkeeps only the first non-empty line/first sentence and applies a 240-character cap before the value is rendered into Markdown/PDF. The requirement says complete prompt-derived titles should be preserved in document content; only the filename needs a filesystem byte limit. Please keep the full public document title and apply truncation only insanitizeExportFilename.
There are also two merge-readiness items:
CONTRIBUTING.mdrequires a finalbehavior -> exact command -> resultevidence mapping, confirmation that checks ran after the last material edit, and uncovered risks. The current validation section lists aggregate results but does not provide that mapping.- The PR currently conflicts with the latest
main(including the recent artifact-download changes), so it needs to be rebased and the sidebar/web-channel conflicts reconciled.
One smaller acceptance-wording mismatch: the generated web API map contains sessions.exportConversation; the HTTP denylist does correctly keep the capability unavailable at runtime. Please either adjust the stated criterion to describe runtime capability filtering or update the map-generation boundary if the map itself must exclude the method.
Resolve conflicts with collapsible workspace panels and selective artifact downloads while preserving desktop-only conversation export.
Problem
Conversation content can currently be downloaded one item at a time, but users cannot export a complete conversation as a portable document. This makes it difficult to archive, share, or reuse a finished research session outside Open Science.
Addresses #486.
Proposed change
Scope and non-goals
Acceptance criteria and validation
All checks below ran after the last material edit, including merge-conflict resolution commit
c228f82:npm test -- --run src/shared/conversation-export.test.ts src/main/session-persistence/conversation-export.test.ts src/main/web-service/http-server.test.ts src/preload/index.test.ts src/renderer/web/api-map.generated.test.ts src/renderer/src/pages/workspace/WorkspaceSidebar.render.test.tsx src/renderer/src/pages/workspace/WorkspacePage.draft-preservation.test.tsx src/renderer/src/pages/workspace/WorkspacePage.preview-panel-resize.test.tsx-> passed, 8 files and 126 tests.npm run typecheck-> passed.npm run lint-> passed with 0 errors; 23 existing warnings are outside this PR diff.npm test-> passed, 574 files passed and 11 skipped; 8,598 tests passed and 139 skipped.npm run check:web-api-map-> passed.npx prettier --check package.json package-lock.json src/main/ipc.ts src/main/session-persistence/conversation-export.test.ts src/main/session-persistence/conversation-export.ts src/main/web-service/http-server.test.ts src/main/web-service/http-server.ts src/preload/index.d.ts src/preload/index.test.ts src/preload/index.ts src/renderer/src/pages/workspace/WorkspacePage.tsx src/renderer/src/pages/workspace/WorkspaceSidebar.render.test.tsx src/renderer/src/pages/workspace/WorkspaceSidebar.tsx src/renderer/web/api-map.generated.ts src/shared/conversation-export.test.ts src/shared/conversation-export.ts-> passed.git diff --check origin/main HEAD-> passed.Uncovered risks: this conflict-resolution pass did not repeat manual native Save As/PDF verification on every supported operating system. The previously posted review findings about active-session status normalization, unmatched reasoning blocks, and prompt-derived document-title truncation remain open for follow-up.
Review focus